Repository navigation
lib: ZonedDateTime.toLocaleString must not accept a timeZone option - #64268
Tech Guy (lukiod) wants to merge 2 commits into
Conversation
The engines reject a timeZone option here because a ZonedDateTime already carries its time zone, and MDN documents that it must not be provided. lib.esnext.temporal typed the options as Intl.DateTimeFormatOptions, so the call type checked. Introduce ZonedDateTimeToLocaleStringOptions, which omits timeZone, and use it for ZonedDateTime.toLocaleString. The existing temporal.ts fixture already marked such a call as WRONG; its baseline now reports it. Fixes microsoft#64227
There was a problem hiding this comment.
🟡 Changes recommended
The new type still accepts timeZone through structurally assignable non-literal option objects.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Restricts Temporal.ZonedDateTime.toLocaleString options and updates compiler baselines.
Changes:
- Adds a dedicated options interface excluding
timeZone. - Updates type and diagnostic baselines.
File summaries
| File | Description |
|---|---|
lib.esnext.temporal.d.ts |
Defines and applies the restricted options type. |
temporal.errors.txt |
Records the new invalid-option diagnostic. |
temporal.types |
Records the updated method signature. |
Review details
- Files reviewed: 2/3 changed files
- Comments generated: 0
- Review effort level: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
|
Regarding Copilot's comment, maybe it's better to define the interface like this interface ZonedDateTimeToLocaleStringOptions extends Intl.DateTimeFormatOptions {
timeZone?: never;
} |
typescript-automation
Bot
added
For Milestone Bug
Omit only applies excess property checking to an object literal, so a variable typed Intl.DateTimeFormatOptions still carried a timeZone into ZonedDateTime.toLocaleString. Declaring the property as never refuses it in both forms, because string is not assignable to never. The added fixture covers the variable form, which is the one the omission did not catch.
|
Adopted, with the variable form covered by a fixture. You were right that the const widened: Intl.DateTimeFormatOptions = { timeZone: "Pacific/Auckland" };
zdt.toLocaleString("de-DE", widened); // accepted under OmitWith
|
Fixes #64227
Temporal.ZonedDateTime.prototype.toLocaleStringwas typed withIntl.DateTimeFormatOptions, so atimeZoneoption type checked even though every engine throws on it (aZonedDateTimealready carries its time zone, and MDN documents that the option must not be provided).This adds
ZonedDateTimeToLocaleStringOptions extends Omit<Intl.DateTimeFormatOptions, "timeZone">next toZonedDateTimeToStringOptionsand uses it forZonedDateTime.toLocaleString, the shape proposed in the issue.PlainDateTimeand the othertoLocaleStringsignatures are unchanged, sincetimeZoneis legal there.The existing
temporal.tsfixture already contains such a call marked/* WRONG */(line 594); with this change it reportsTS2353, so the accepted baseline changes are that new error plus the updated signature intemporal.types.Verified with
go -C ./tsc test -run='TestLocal/temporal' ./internal/testrunnerafter accepting the baselines, then the fullgo -C ./tsc test ./internal/testrunner(ok, 64.6s). Also checked against thetypescript@7.1.0-dev.20260913.1nightly by swapping in the patched lib: the illegal call errors,dateStyle/timeZoneNameoptions and aPlainDateTime.toLocaleStringcall withtimeZonestill compile.AI assistance: the patch and test run were drafted with Claude Code; I chose the issue, reviewed the diff and baselines, and will handle review feedback myself.